tools/upmap: upmap-remapped.py correctness and error handling fixes - #51
Open
dvanders wants to merge 12 commits into
Open
tools/upmap: upmap-remapped.py correctness and error handling fixes#51dvanders wants to merge 12 commits into
dvanders wants to merge 12 commits into
Conversation
gen_upmap() had three bugs: it removed reverse pairs such as (314, 272) & (272, 314) while iterating over the list holding them, so it missed some; its bubble sort never terminated when the mappings formed a cycle, hanging the script with no output; and dropping the mapping of an osd which is out left the remaining ones free to map onto an osd which is already in the up set, which the mon ignores, so the pg stayed remapped forever. Build the replicated mappings from the up and acting sets directly, and on erasure-coded pools drop what the mon would ignore, then order the rest by walking the chains the mappings form. A cycle has no valid order and is left out, which covers the reverse pairs as well. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The four places which read something from the cluster each repeated the same if use_shell / else mon_command dance. No change in behaviour. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The 'jq -r .' pipes are no-ops left over from when the script read commands with subprocess.getoutput(), which folds stderr into the json. They also hid failures, because with shell=True check=True only sees the exit status of jq, which is 0 on empty input. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A command the mon refused was treated as empty output. 'osd pool ls detail' is read as text, so a failure there left pool_type empty and the script died with a KeyError on the first remapped pg. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Five seconds is not enough for 'pg ls remapped' or 'osd dump' on a large cluster with many remapped pgs, which is what this script is for. A librados error is not a ValueError either, so it came out as a traceback instead of 'Error loading ...', and so did CalledProcessError in shell mode. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
'pool' in line also matches the snapshot lines of the listing, so a pool snapshot with 'pool' in its name overwrites the type of its pool with a date, and the script exits with 'Unknown pool type' having done nothing. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A pool which was deleted between reading the pgs and reading the pools ended the run with a KeyError from pool_type[pool]. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Pass the replicated flag rather than duplicating the gen_upmap() call, and make has_upmap the set it wants to be. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A bare except also catches KeyboardInterrupt, and the two around the gen_upmap() calls turned any bug inside it into a silently skipped pg. Name what is expected at each of the three sites; gen_upmap() now returns no mappings when up and acting differ in length, which also survives python -O. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
crush_weight() walked the whole 'osd df' output for every shard of every remapped pg. Building the mappings for 30000 remapped 8+3 pgs on a 2000 osd cluster goes from 1.94s to 0.03s, with identical output. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Anything which was not exactly '--ignore-backfilling' was ignored, so a mistyped option quietly did a full production run. Parsing before connecting also lets --help work without a cluster. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
cluster.shutdown() was only reached by falling off the end of the script, so none of the sys.exit() paths ran it, including 'There are no remapped PGs'. atexit covers them all and still leaves shell mode alone. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A review of
upmap-remapped.pyturned up three ways it can fail to do its job,plus a handful of smaller issues. One fix per commit, so they can be taken or
dropped individually.
Bugs
erasure-coded pools never terminates when the mappings form a cycle, e.g. up
[4,5,3]against acting[3,4,5]. Because stdout is block buffered when pipedinto
sh, the commands generated before the hang never run either, so the rundoes nothing at all.
time when an osd is out or gone, which leaves the remaining ones free to map
onto an osd that is already in the up set. The mon ignores such a mapping, so
the pg stays remapped no matter how many times the script is run. With osd.7
out, up
[1,2]and acting[2,7]producedpg-upmap-items 1 2, a no-op.(314, 272)&(272, 314)mutated the list it was iterating over, so pairsafter the first were left in.
poolin its name is parsed as a pool definition andoverwrites that pool's type, after which the script exits with
Unknown pool typehaving done nothing.a
KeyError.osd pool ls detailleft the pool types empty and the script died with a
KeyErroron the first pg.pg ls remappedon a large clusterwith many remapped pgs, and the resulting librados error was not caught.
--ignore-backfillingwas ignored, so a typoquietly did a full production run.
Also
jqis no longer required; thejq -r .pipes were no-ops that also maskedfailing
cephcommands, sincecheck=Trueonly saw jq's exit status.30000 remapped 8+3 pgs on a 2000 osd cluster goes from 1.94s to 0.03s.
if use_shell / else mon_commandblocks became one helper, bareexcept:clauses were narrowed, and the librados connection is now closed onevery exit path rather than only the last one.
Testing
gen_upmap()was checked against a simulation of the mon'spg-upmap-itemshandling over 120000 random up/acting combinations, including out osds, unknown
osds and missing erasure-coded shards. The old code produced mappings the mon
would ignore in ~28000 of them and hung in ~3800; the new code has none of
either, and fixes slightly more shards because a pg containing a cycle now still
gets its other mappings. Both the shell and librados paths were run end to end
against a stub cluster.
Worth knowing
whose pool is missing is now skipped with a message, since that is a benign
race.